-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rageshake: Add file server with basic auth #3169
Conversation
Set via environment variables on startup.
scripts/rageshake.go
Outdated
if usr == "" || pass == "" { | ||
fmt.Println("BUGS_USER and BUGS_PASS env vars not found. No authentication is running for /api/listing") | ||
} else { | ||
fs = basicAuth(fs, usr, pass, "Enter username and password") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't realm normally something like Riot bug reports
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an arbitrary string which is sent to the client in a popup, which is usually prefixed with "This website says: REALM" hence the request to enter u/p. I don't care what this is called, so I'll change it to whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's more than that. It is specifically used to identify things which should have the same u/p on a domain. Obviously if different apps with different auth all use "Enter username and password" that's no good.
Also, some clients may show "Enter the username and password for <realm>".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/rageshake.go
Outdated
} else { | ||
fs = basicAuth(fs, usr, pass, "Enter username and password") | ||
} | ||
http.Handle("/api/listing/", fs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so a link to an individual report will be /api/listing/2017-02-07/205222
? How about /reports
instead of /api/listing
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the link that will be present for our deployment. The Apache is configured to route riot.im/bugreports/*
to /api/*
so the individual report is linked as https://riot.im/bugreports/listing/2017-02-07/205222
. I can rename listing
to reports
if you want to be redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. so why /api
then? Why not just /submit
and /listing
(or /reports
).
I suddenly realise I don't actually care about this, though. Do as you see fit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We as a team have certain conventions when it comes to Go and HTTP servers. One of them is that everything public is served under /api
so that you can attach Prometheus metrics (which hook into /metrics
) and pprof tooling (which hook into /debug/pprof
) without making those endpoints visible to the wider internet which would be the case if we attached to /
. See https://docs.google.com/document/d/1yu7qImAcr-_yN72Ndb5ht_zjyK0hxXQGZQYIKFaGBXw/edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of things
lgtm |
Set via environment variables on startup.